Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editor Array subhints fix #32871

Closed

Conversation

lonegamedev
Copy link

F.e restrict Array's editor UI to allow only NodePaths of RigidBody type.

I think hint_string syntax was supposed to be:

(VARIANT_ID_INT):[(HINT_ID_INT)/(HINT_STRING)]
the section after colon is optional

but it's hard to figure out without any docs,
and broken string parser.

Use Example:

ADD_PROPERTY(PropertyInfo(Variant::ARRAY,
"hints_array", PROPERTY_HINT_NONE,
itos(Variant::NODE_PATH)+":"+itos(PROPERTY_HINT_NODE_PATH_VALID_TYPES)+"/RigidBody"),
"set_hints", "get_hints");

@Chaosus
Copy link
Member

Chaosus commented Oct 17, 2019

Thanks for the contribution. You need to squash the commits together (as described in http://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#mastering-the-pr-workflow-the-rebase).

…g syntax).

F.e restrict Array's editor UI to allow only NodePaths of RigidBody type.

I think hint_string syntax was supposed to be:

(VARIANT_ID_INT):[(HINT_ID_INT)/(HINT_STRING)]
the section after colon is optional

but it's hard to figure out without any docs,
and broken string parser.

Use Example:

ADD_PROPERTY(PropertyInfo(Variant::ARRAY,
"hints_array", PROPERTY_HINT_NONE,
itos(Variant::NODE_PATH)+":"+itos(PROPERTY_HINT_NODE_PATH_VALID_TYPES)+"/RigidBody"),
"set_hints", "get_hints");
@lonegamedev
Copy link
Author

Sure - no problem. Just squashed my commits.

@akien-mga akien-mga requested a review from bojidar-bg October 23, 2019 08:47
@akien-mga
Copy link
Member

akien-mga commented Oct 23, 2019

For the reference, so far there seems to be only three uses of this undocumented feature:

servers/physics_server.cpp
244:    ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "exclude", PROPERTY_HINT_NONE, itos(Variant::_RID) + ":"), "set_exclude", "get_exclude");

servers/physics_2d_server.cpp
251:    ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "exclude", PROPERTY_HINT_NONE, itos(Variant::_RID) + ":"), "set_exclude", "get_exclude");

scene/gui/rich_text_label.cpp
2720:   ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "custom_effects", PROPERTY_HINT_RESOURCE_TYPE, "17/17:RichTextEffect", (PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_SCRIPT_VARIABLE), "RichTextEffect"), "set_effects", "get_effects");

The syntax is quite non-obvious.

It was implemented by @bojidar-bg in #6930, fixing #3586.

Are you sure that those changes don't break the GDScript export hints?

@lonegamedev
Copy link
Author

Thank you for looking into it.

So I did check use-cases you have mentioned - and my fix crashes editor for one of them (resource hint), and doesn't fix every other case (sub-property hint).

I think we need to step back and re-think it a bit - since we have more than one format here.

Summary:

ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "hints_array", PROPERTY_HINT_NONE, itos(Variant::NODE_PATH)+":"+itos(PROPERTY_HINT_NODE_PATH_VALID_TYPES)+"/RigidBody"), "set_hints", "get_hints");

master: sub-hint not working
my fix: works as expected

ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "hints_array2", PROPERTY_HINT_NONE, itos(Variant::NODE_PATH)+":"), "set_hints2", "get_hints2");

master: works as expected
my fix: works as expected

ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "hints_array3", PROPERTY_HINT_NONE, itos(Variant::NODE_PATH)+":Path2D/PathFollow2D/Sprite:texture:size"), "set_hints3", "get_hints3");

master: sub-hint not working
my fix: sub-hint not working

I need to look into it more - colon is used in more than one context. It seems, the first colon is hint/sub-hint separator - and any following colon is property descriptor - but I can't confirm this easily.

ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "hints_array4", PROPERTY_HINT_NONE, itos(Variant::_RID) + ":"), "set_hints4", "get_hints4");

master: can't add any element to this array via editor
my fix: can't add any element to this array via editor
but I guess this is correct behavior, since RID's are not(?) persistent

ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "hints_array5", PROPERTY_HINT_RESOURCE_TYPE, "17/17:RichTextEffect", (PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_SCRIPT_VARIABLE), "RichTextEffect"), "set_hints5", "get_hints5");

master: works as expected
my fix: editor crashes :(

ERROR: Variant::construct: Index p_type=1717 out of size (VARIANT_MAX=27)

The slash left of colon is probably what original code expected.

If I understand this correctly - we can have sub-types left of the colon sign so:
17 = Variant::Object
17 = Resource(?) I'm not sure where it comes from.

Could you confirm this?

Still I think this case can be fixed fairly easily.
btw. perhaps we should create some kind of C++ macro for generating hint strings? This would be a good point of reference.

@bojidar-bg
Copy link
Contributor

Hey, sorry for the late comment.

Looking at what I implemented in GDScript and what the previous code was, seems like the format is:
itos(Type(sub_type)) + "/" + itos(PropertyHint(sub_hint)) + ":" + String(sub_hint_string)
or, when not needing a hint:
itos(Type(sub_type)) + ":" + String(sub_hint_string)

While the examples given the above comment (and the new code) expect:
itos(Type(sub_type)) + ":" + itos(PropertyHint(sub_hint)) + "/" + String(sub_hint_string)
or, when not needing a hint:
itos(Type(sub_type)) + ":" + String(sub_hint_string)

Now, in general, the first option should be better since we already require that the colon is there, but the "/" can be ambiguous in the second case (and thus, if the sub_hint_string includes a slash, you would need to add "0/" before it)

@akien-mga akien-mga modified the milestones: 3.2, 4.0 Dec 13, 2019
@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Dec 13, 2019
@aaronfranke
Copy link
Member

@lonegamedev Is this still desired? If so, it needs to be rebased on the latest master branch.

@YuriSizov YuriSizov requested review from a team and removed request for bojidar-bg August 24, 2021 22:24
@akien-mga
Copy link
Member

Closing as this is stale. This use case is likely better handled in 4.0 using typed arrays, but this can be reopened if there is still a need for what it implemented.

@akien-mga akien-mga closed this Sep 20, 2022
@akien-mga akien-mga added archived and removed cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants